refactor: clean up aggregation planning - #25104
Conversation
| required_input_ordering: Option<OrderingRequirements>, | ||
| /// Describes how the input is ordered relative to the group by columns | ||
| /// | ||
| /// This field is also overloaded to mean "the output MUST preserve this |
There was a problem hiding this comment.
This is an existing assumption, and I feel it's slightly confusing.
There was a problem hiding this comment.
Do you think we should make a separate flag for output order?
There was a problem hiding this comment.
Not sure, maybe 2-flag combination can be more confusing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25104 +/- ##
==========================================
- Coverage 81.80% 81.80% -0.01%
==========================================
Files 1130 1130
Lines 417754 417701 -53
Branches 417754 417701 -53
==========================================
- Hits 341754 341698 -56
+ Misses 55875 55874 -1
- Partials 20125 20129 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks great to me -- thank you @2010YOUY01
| required_input_ordering: Option<OrderingRequirements>, | ||
| /// Describes how the input is ordered relative to the group by columns | ||
| /// | ||
| /// This field is also overloaded to mean "the output MUST preserve this |
There was a problem hiding this comment.
Do you think we should make a separate flag for output order?
| // order promise. See its comment for details. | ||
| use AggregateMode::*; | ||
| use InputOrderMode::*; | ||
| let stream = match (self.mode, &self.input_order_mode) { |
There was a problem hiding this comment.
I really like how this uses the match arm to ensure all combinations are properly covered
3 participants
Which issue does this PR close?
Rationale for this change
See issue
What changes are included in this PR?
What is the testing strategy for this PR?
Existing tests.
Are there any user-facing changes?
No.